Skip to content

Constify implementations of (Try)From for int types#86840

Merged
bors merged 3 commits into
rust-lang:masterfrom
usbalbin:const_from
Aug 11, 2021
Merged

Constify implementations of (Try)From for int types#86840
bors merged 3 commits into
rust-lang:masterfrom
usbalbin:const_from

Conversation

@usbalbin

@usbalbin usbalbin commented Jul 3, 2021

Copy link
Copy Markdown
Contributor

I believe this to be one of the (many?) things blocking const (Range) iterators.

If this is to be merged maybe that should wait until #![feature(const_trait_impl)] no longer needs #![allow(incomplete_features)]? - Done

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jonas-schievink jonas-schievink added the F-const_trait_impl `#![feature(const_trait_impl)]` label Jul 3, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@usbalbin usbalbin changed the title [experiment] constified implementations of (Try)From for number types Constify implementations of (Try)From for int types Jul 4, 2021
@bors

bors commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #87768) made this pull request unmergeable. Please resolve the merge conflicts.

Comment thread library/core/tests/lib.rs Outdated
@oli-obk

oli-obk commented Aug 7, 2021

Copy link
Copy Markdown
Contributor

r? @oli-obk

Please remove the commented out attribute and create a tracking issue, then we can merge this

@rust-highfive rust-highfive assigned oli-obk and unassigned m-ou-se Aug 7, 2021
@usbalbin usbalbin marked this pull request as ready for review August 7, 2021 15:38
@usbalbin

usbalbin commented Aug 7, 2021

Copy link
Copy Markdown
Contributor Author

Ok, on it

@usbalbin

usbalbin commented Aug 7, 2021

Copy link
Copy Markdown
Contributor Author

Tracking issue added and code updated. I also added a test specific to floats since that will also be enabled by this

@oli-obk

oli-obk commented Aug 9, 2021

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Aug 9, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit c8bf5ed has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2021
assert_eq!(FROM, 1i64);

// From int to float
const FROM_F64: f64 = f64::from(42u8);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rust-lang/wg-const-eval this is an interesting edge case around floats. We forbid floats in const fn, but those checks only happen at the declaration site, and libcore uses the feature gate that allows them. So now we can invoke const fns (only within items, not within other const fn) that work with floats.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should really work towards getting rid of "things that are allowed in const but forbidden in const fn"...

@bors

bors commented Aug 9, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit c8bf5ed with merge 8bb9ff63ed0125748c04b13a40ef468b97b84708...

@bors

bors commented Aug 9, 2021

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 9, 2021
@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@oli-obk

oli-obk commented Aug 9, 2021

Copy link
Copy Markdown
Contributor

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 10, 2021
Constify implementations of `(Try)From` for int types

I believe this to be one of the (many?) things blocking const (Range) iterators.

~~If this is to be merged maybe that should wait until `#![feature(const_trait_impl)]` no longer needs `#![allow(incomplete_features)]`?~~ - Done
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 10, 2021
Constify implementations of `(Try)From` for int types

I believe this to be one of the (many?) things blocking const (Range) iterators.

~~If this is to be merged maybe that should wait until `#![feature(const_trait_impl)]` no longer needs `#![allow(incomplete_features)]`?~~ - Done
($Small: ty, $Large: ty, #[$attr:meta], $doc: expr) => {
#[$attr]
impl From<$Small> for $Large {
#[rustc_const_unstable(feature = "const_num_from_num", issue = "87852")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stability attributes do not usually work on trait impls. Do they work for impl const?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. See this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That tests the case where we call a trait fn on a concrete type.

But there is another way to use a trait impl: to satisfy a bound.

const fn const_context_generic<T: MyTrait>() {
  T::func();
}

const_context_generic::<Unstable>();

Is that properly checked?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That tests the case where we call a trait fn on a concrete type.

But there is another way to use a trait impl: to satisfy a bound.

const fn const_context_generic<T: MyTrait>() {
  T::func();
}

const_context_generic::<Unstable>();

Is that properly checked?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, those are probably not. I guess it is fine to let this PR pass and I will come up with a fix in the future. (and we need to wait for #87375, as that introduces a check on const bounds to error on previous code that should not be accepted) Trait bounds on const functons are gated behind a feature anyways, so even if this would allow one to use an unstable library feature without having it enabled, it would still require users to use a nightly compiler anyways.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2021
Rollup of 14 pull requests

Successful merges:

 - rust-lang#86840 (Constify implementations of `(Try)From` for int types)
 - rust-lang#87582 (Implement `Printer` for `&mut SymbolPrinter`)
 - rust-lang#87636 (Added the `Option::unzip()` method)
 - rust-lang#87700 (Expand explanation of E0530)
 - rust-lang#87811 (Do not ICE on HIR based WF check when involving lifetimes)
 - rust-lang#87848 (removed references to parent/child from std::thread documentation)
 - rust-lang#87854 (correctly handle enum variants in `opt_const_param_of`)
 - rust-lang#87861 (Fix heading colours in Ayu theme)
 - rust-lang#87865 (Clarify terms in rustdoc book)
 - rust-lang#87876 (add `windows` count test)
 - rust-lang#87880 (Remove duplicate trait bounds in `rustc_data_structures::graph`)
 - rust-lang#87881 (Proper table row formatting in platform support)
 - rust-lang#87889 (Use smaller spans when suggesting method call disambiguation)
 - rust-lang#87895 (typeck: don't suggest inaccessible fields in struct literals and suggest ignoring inaccessible fields in struct patterns)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3b41447 into rust-lang:master Aug 11, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 11, 2021
@usbalbin

Copy link
Copy Markdown
Contributor Author

Any suggestions for what to put under Public APIin the tracking issue? :)

@oli-obk

oli-obk commented Aug 28, 2021

Copy link
Copy Markdown
Contributor

I don't think the default libs api issue template applies here.

@RalfJung

Copy link
Copy Markdown
Member

I'd probably put the impl headers of the new impls.

@usbalbin

Copy link
Copy Markdown
Contributor Author

I'd probably put the impl headers of the new impls.

There is quite a massive amount of impls :)

also note that I have not added any impls just constified the existing ones

@RalfJung

Copy link
Copy Markdown
Member

Yeah, with these macros there's a lot of them... maybe there is a good way to summarize them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-const_trait_impl `#![feature(const_trait_impl)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants